-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468
Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468
Conversation
22b411a
to
af8073b
Compare
af8073b
to
bddd0b1
Compare
@@ -92,7 +92,7 @@ def init_unversioned_attrs(attrs, model) | |||
# @api private | |||
def reify_attribute(k, v, model, version) | |||
if model.has_attribute?(k) | |||
model[k.to_sym] = v | |||
model[k.to_sym] = v unless model.class.readonly_attribute?(k.to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty dangerous since it silently throws away assignments of readonly attributes without the developer knowing about it. This is the exact problem that config.active_record.raise_on_assign_to_attr_readonly was looking to address.
I see some viable options:
- We consider paper_trail's reify operation as a "dangerous" operation that automatically disable
attr_readonly
declarations (somehow) and forces the attributes to be rewritten. - We take the stance you can't have both worlds. As in, you can't reify a record if it's going to set a readonly attribute. This is basically not changing anything.
- We only set the value iff it's a different value. This allows the reify to work in some cases and fail (loudly) in others. This seems a little odd to me, but still, it's an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only set the value iff it's a different value. This allows the reify to work in some cases and fail (loudly) in others. This seems a little odd to me, but still, it's an option.
This is the approach in #1473. Any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked about this with someone today and came to the conclusion that option 1 is actually the best. Option 3 happens to work sometimes for folks. However, if you consider the business logic decision to add attr_readonly
to an attribute at a certain/later point in time now affects your ability to reify
a record.
In my opinion, reify
ing a record is a break-the-glass operation that should just always work (similar to update_column
). At the very least, the assignment should just always work. Devs can then call save(validate: false)
if they want to persist the an invalid record.
As for the implementation of option 1, I think papertrail will have to do some tricks to temporarily empty out ([]
) the readonly array for a model while it's doing it assignment for the reify. Or, it can use private apis to set the attributes directly, bypassing the raise
. Or, maybe Rails will take a patch to allow a bypass block similar to no_touching
.
Should we close this in favor of #1473? |
Yes, I agree. |
Fixes #1467
Check the following boxes:
master
(if not - rebase it).code introduces user-observable changes.
and description in grammatically correct, complete sentences.
I manually tested this PR with these commands: